Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

standardize the notation for optional parameters #698

Merged
merged 4 commits into from
Feb 29, 2024
Merged

standardize the notation for optional parameters #698

merged 4 commits into from
Feb 29, 2024

Conversation

kartikeyakirar
Copy link
Contributor

fixes #674

@m7pr m7pr mentioned this pull request Feb 28, 2024
Closed
m7pr
m7pr reviewed Feb 28, 2024
View reviewed changes
R/tm_a_regression.R Outdated Show resolved Hide resolved
vedhav
vedhav reviewed Feb 28, 2024
View reviewed changes
R/tm_a_pca.R Outdated Show resolved Hide resolved
@chlebowa
Copy link
Contributor

chlebowa commented Feb 28, 2024 •
edited
Loading

Just a quick note. The fact that an argument is optional is readily apparent in the function definition (no correction: has a default value) so there is no need to use the word ever time. We don't always state the default value in the argument description, after all, only when it matters.

@m7pr m7pr mentioned this pull request Feb 28, 2024
Closed
14 tasks
@m7pr
Copy link
Contributor

m7pr commented Feb 28, 2024

If we are sure that the parameter does not get set inside functions body to anything (if it's NULL/empty during the call) then maybe it's a good idea to remove optional completely.

@kartikeyakirar
Copy link
Contributor Author

kartikeyakirar commented Feb 28, 2024 •
edited
Loading

The fact that an argument is optional is readily apparent in the function definition (no default value)

I am confused, Arguments are optional when you do not have to specify a value for them. They can have a default value, which is used if you do not specify. (did i read it wrong)

@chlebowa
Copy link
Contributor

@kartikeyakirar You are correct, my brain flipped the logic.

@kartikeyakirar
@m7pr suggestion
c9ccb2c
@kartikeyakirar
merge back from pre-release
fb7b3e5 
Merge branch 'pre-release@main' into optional_notation@pre-release@main

# Conflicts:
#	man-roxygen/ggplot2_args_multi.R
#	man/tm_a_pca.Rd
#	man/tm_a_regression.Rd
#	man/tm_g_association.Rd
#	man/tm_g_distribution.Rd
#	man/tm_missing_data.Rd
#	man/tm_outliers.Rd
@kartikeyakirar kartikeyakirar merged commit e5fc4c9 into pre-release@main Feb 29, 2024
@kartikeyakirar kartikeyakirar deleted the optional_notation@pre-release@main branch February 29, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vedhav vedhav vedhav left review comments

@m7pr m7pr m7pr approved these changes

Assignees

@m7pr m7pr

Labels
core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kartikeyakirar @chlebowa @m7pr @vedhav